Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve pull_request_template.md #22888

Merged
merged 7 commits into from
Feb 21, 2023
Merged

Improve pull_request_template.md #22888

merged 7 commits into from
Feb 21, 2023

Conversation

wolfogre
Copy link
Member

@wolfogre wolfogre commented Feb 13, 2023 •
edited
Loading

Update pull_request_template.md because:

  • It's a kind idea to hide the tips. However, it's easier to include them in the commit message by mistake when you cannot see them. Check git log | grep 'Please check the following:'. So don't hide it, expose it and help fix it.
  • "for backports" is much clearer than "for bug fixes". I saw someone post a PR to a release branch because they believed it was the right way for a bugfix.
  • "Allow edits by maintainers", or we have to ask the contributor to update the branch and they could be confused.
  • Remind the contributor that the words could be included in the commit message, to avoid some words like "Hello", "Sorry". If they really need them, they can separate them with a line, like:
Close #xxxx
Because ... Then ... Finally ...
---
Hello, this is my first time opening a pull request. Sorry for any mistakes.

And the merger should be careful, check and delete the extra content before merging.

@wolfogre wolfogre added type/docs This PR mainly updates/creates documentation skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Feb 13, 2023
@yardenshoham
Copy link
Member

I don't really like that --- rule. I'd prefer that info in the first PR comment.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 13, 2023
@wolfogre
Copy link
Member Author

wolfogre commented Feb 13, 2023 •
edited
Loading

I don't really like that --- rule. I'd prefer that info in the first PR comment.

Why not?

If contributors don't like it, they don't have to use it, and they can still write things in the first comment.

If mergers don't like it, it's probably because it requires them to merge more carefully. And that is indeed my evil plan!😂

Just kidding, I'll update it to "the first comment" if most people don't like ---.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 14, 2023
yardenshoham
yardenshoham suggested changes Feb 14, 2023
View reviewed changes
Copy link
Member

@yardenshoham yardenshoham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do keep the --- change (which, again, I am against). You'll have to make changes in at least one other place,

gitea/CONTRIBUTING.md

Line 266 in 7b5b739

with the rest of the summary matching the original PR. Similarly for frontports

The backports today take the original PR's summary

@wolfogre
Copy link
Member Author

wolfogre commented Feb 14, 2023 •
edited
Loading

If you do keep the --- change (which, again, I am against). You'll have to make changes in at least one other place,

gitea/CONTRIBUTING.md

Line 266 in 7b5b739

with the rest of the summary matching the original PR. Similarly for frontports

The backports today take the original PR's summary

I haven't thought about it, because I always send backports like git cherry-pick [the commit id which has been merged to main], and the commit message (I believe it has been corrected) will be the body of new PR automatically, and the only thing I have to do is just add "Backport #xxx".

I don't know, maybe I'm not doing it the usual way. Whatever, if it turns out that the merger needs to be more careful, I'm fine with that. Even without ---, the merger still has to check it again if the contributor keeps the deleted text in backport PR. However, if contributors know how to use --- in the right way, it will be easier to check.

Maybe we can have a robot to send backport PRs automatically, and it's easy to recognize --- and handle it. I heard you already did something like that. 👀

@yardenshoham
Copy link
Member

When I did it I followed these instructions so when I create the PR I literally concatenate them:
https://github.com/yardenshoham/gitea-backporter/blob/5ebdca217f8b22c18951e0bef014d60a7bc7679c/src/github.ts#L80

@wolfogre
Copy link
Member Author

When I did it I followed these instructions so when I create the PR I literally concatenate them: yardenshoham/gitea-backporter@5ebdca2/src/github.ts#L80

You are right to follow the instructions, however, if the merger has adjusted the commit message and your awesome backporter uses the originalPr.body to send a backport, the merger has to adjust it again. That's not your fault, that's the merger's work, if your backporter removes ---, that's a friendly help. If your backporter keeps it, it still does nothing wrong.

BTW, could you please add a delay to backporter? I mean just help sending backports only for PRs which have been forgotten for maybe 24 hours, to avoid conflicts. Never mind if you already did.😄

@wxiaoguang
Copy link
Contributor

I agree with the "---" separator, or anything similar to help to distinguish commit-message and none-commit-message. It just helps the mergers to know which part is important while which part is not.

Mergers should always pay attention to the merge messages, and improve the message manually if necessary, instead of just clicking "Merge" without any check. Clicking "merge" without any check is the easiest work without any requirement, mergers should do far more than that. I have said many times in discord channel that the commit message was polluted, but various unrelated messages still go into git history.

@yardenshoham
Copy link
Member

@wolfogre

No problem, I'll change my code to match whatever behavior is written in CONTRIBUTING.md but you have to agree that before this PR lands, there must be changes to CONTRIBUTING.md

Thanks for calling my backporter awesome!

@wolfogre
Copy link
Member Author

... I'll change my code to match whatever behavior is written in CONTRIBUTING.md but you have to agree that before this PR lands, there must be changes to CONTRIBUTING.md

TBH, I don't really know how to update CONTRIBUTING.md, it's clear and correct to say "with the rest of the summary matching the original PR." It's the merger's job to remove the ---, not the backport sender. There's nothing wrong with keeping the --- in a backport PR.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 21, 2023
@techknowlogick techknowlogick merged commit 97aacc3 into go-gitea:main Feb 21, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 21, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
5386697 
* upstream/main: (23 commits)
  add margin top to the top of branches (go-gitea#23002)
  Add me to maintainers (go-gitea#23026)
  Render access log template as text instead of HTML (go-gitea#23013)
  Use `gt-relative` class instead of the ambiguous `gt-pr` class  (go-gitea#23008)
  Fix intermittent panic in notify issue change content (go-gitea#23019)
  Improve pull_request_template.md (go-gitea#22888)
  Hide 2FA status from other members in organization members list (go-gitea#22999)
  handle deprecated settings (go-gitea#22992)
  Add scopes to API to create token and display them (go-gitea#22989)
  Remove unnecessary and incorrect `find('.menu').toggle()` (go-gitea#22987)
  Improve issues.LoadProject (go-gitea#22982)
  Add 1.18.4 changelog (go-gitea#22991) (go-gitea#22995)
  Fix pull request branch selector visible without clicking Edit (go-gitea#23012)
  Bump golang.org/x/net from 0.4.0 to 0.7.0 (go-gitea#22980)
  Fix panic when call api (/repos/{owner}/{repo}/pulls/{index}/files) (go-gitea#22921)
  only trigger docs build and publish when docs changed (go-gitea#22968)
  Get rules by id when editing branch protection rule (go-gitea#22932)
  Fix hidden commit status on multiple checks (go-gitea#22889)
  Add me to maintainers (go-gitea#22998)
  Add all units to the units permission list in org team members sidebar (go-gitea#22971)
  ...
@yardenshoham yardenshoham mentioned this pull request Feb 22, 2023
Open
@yardenshoham
Copy link
Member

Again, why I think the first comment is better than --- 443dcc2

@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yardenshoham yardenshoham yardenshoham requested changes

@techknowlogick techknowlogick techknowlogick approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/docs This PR mainly updates/creates documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wolfogre @yardenshoham @wxiaoguang @lunny @techknowlogick @GiteaBot